Tolerate failure #183

yaauie · 2022-10-20T19:04:12Z

When an Event cannot be created directly from the hit, or when the
docinfo cannot be merged into a non-hash field in the hit, emit an
Event tagged with _elasticsearch_input_failure that contains the
JSON-encoded hit in [event][original] instead of crashing.

Resolves #182

yaauie · 2022-10-20T19:09:09Z

docs/index.asciidoc

+
+Common causes are:
+
+ - When the hit result contains top-level fields that are {logstash-ref}/processing.html#reserved-fields[reserved in Logstash] but do not have the expected shape. Use the <<plugins-{type}s-{plugin}-target>> directive to avoid conflicts with the top-level namespace.


@karenzone can I get 👀 on this one to make sure I'm not risking breaking the docs build?

It looks like we added {logstash-ref}/processing.html#reserved-fields in 7.7 (it doesn't exist on 7.6 and before), but is present on all of the versions of the reference available in the quick picker (master, current, 8.5, 8.4, 7.17).

It looks like we added {logstash-ref}/processing.html#reserved-fields in 7.7 (it doesn't exist on 7.6 and before), but is present on all of the versions of the reference available in the quick picker (master, current, 8.5, 8.4, 7.17).

The gemlock release file controls which plugin versions get mapped/picked up for each stack version when we run docgen. For that reason, the plugin<->stack versioning generally works itself out without any manual intervention from us.

karenzone

Left some minor wording tweaks inline for your consideration and addressed versioning question. Otherwise, LGTM

TL;DR. The versioning should work itself out and your linking in spot on.

docs/index.asciidoc

karenzone · 2022-10-25T23:22:11Z

docs/index.asciidoc

+
+Common causes are:
+
+ - When the hit result contains top-level fields that are {logstash-ref}/processing.html#reserved-fields[reserved in Logstash] but do not have the expected shape. Use the <<plugins-{type}s-{plugin}-target>> directive to avoid conflicts with the top-level namespace.


It looks like we added {logstash-ref}/processing.html#reserved-fields in 7.7 (it doesn't exist on 7.6 and before), but is present on all of the versions of the reference available in the quick picker (master, current, 8.5, 8.4, 7.17).

The gemlock release file controls which plugin versions get mapped/picked up for each stack version when we run docgen. For that reason, the plugin<->stack versioning generally works itself out without any manual intervention from us.

karenzone

Signing off on the docs portion in case you're waiting for an approval to move forward.

When SECURE_INTEGRATION is speicified, the (non-secure) `:integration` specs are excluded, so we cannot have the `:secure_integration` specs wrapped in a context flagged as `:integration`.

In order for the `ca_trusted_fingerprint` specs to work with the CA's fingerprint, ES needs to be configured to present a cert chain that includes the CA.

When an Event cannot be created directly from the hit, or when the docinfo cannot be merged into a non-hash field in the hit, emit an Event tagged with `_elasticsearch_input_failure` that contains the JSON-encoded hit in `[event][original]` instead of crashing.

donoghuc · 2024-12-30T22:57:39Z

lib/logstash/inputs/elasticsearch.rb

+  # @param scroll_id [String]: a scroll id to resume
+  # @return [Array(Boolean,String)]: a tuple representing whether the response
+  #
+  def process_next_scroll(output_queue, scroll_id)


I cant quite figure out how this is used.

donoghuc · 2024-12-30T23:02:44Z

lib/logstash/inputs/elasticsearch.rb

+  rescue => e
+    serialized_hit = hit.to_json
+    logger.warn("Event creation error, original data now in [event][original] field", message: e.message, exception: e.class, data: serialized_hit)
+    return event_factory.new_event('event' => { 'original' => serialized_hit }, 'tags' => ['_elasticsearch_input_failure'])


Suggested change

return event_factory.new_event('event' => { 'original' => serialized_hit }, 'tags' => ['_elasticsearch_input_failure'])

event_factory.new_event('event' => { 'original' => serialized_hit }, 'tags' => ['_elasticsearch_input_failure'])

Nit: Shouldnt need the explicit return, but I see that is not the only instance in this project.

It is technically redundant, but present for readability.

The explicit return statement makes clear to me that we are intentionally returning that object from the rescue... block, because otherwise when I'm reading code I don't know whether the return value matters to the caller(s).

It's a side-effect of using exceptions for flow-control (which is less than ideal, but it is what we are already starting with).

donoghuc · 2024-12-30T23:08:11Z

lib/logstash/inputs/elasticsearch.rb

+    set_docinfo_fields(hit, event) if @docinfo
+
+    event
+  rescue => e


Suggested change

rescue => e

rescue RuntimeError => e

Looks like set_docinfo_fields explicitly raises a RuntimeError now? May be best to be as specific as possible with error catching.

Yes, but this isn't the only exception we're handling here.

We know that EventFactory#new_event (which delegates to Event::new) can give us exceptions too, specifically when the payload uses reserved fields in incorrect ways. We want to catch any situation in which we fail to create an event from the hit.

donoghuc · 2024-12-30T23:13:47Z

spec/fixtures/test_certs/renew.sh

+cat es.crt ca.crt > es.chain.crt
+
+# output ISO8601 timestamp to file
+date -Iseconds > GENERATED_AT


I feel like expiration date would be more useful 🤷 or both.

openssl x509 -enddate -noout -in es.crt | sed 's/notAfter=//'

donoghuc · 2024-12-30T23:17:08Z

lib/logstash/inputs/elasticsearch.rb

+
+    event
+  rescue => e
+    serialized_hit = hit.to_json


I'm assuming if these were not json serializable in the first place we would never get this far?

Ehhh, we deserialized the hit from json, so it should be able to go back?

If we can't, we are at least not creating a new failure scenario (merely failing to handle something that currently already crashes the plugin).

donoghuc

Test cleanup looks good (one small suggestion on cert generation script).

Release prep looks correct (version number is good and changelog looks accurate)

Had a couple nit comments and a question about potential unused code. Other than that this looks like a good improvement.

yaauie · 2024-12-31T00:32:30Z

lib/logstash/inputs/elasticsearch.rb

+  rescue => e
+    serialized_hit = hit.to_json
+    logger.warn("Event creation error, original data now in [event][original] field", message: e.message, exception: e.class, data: serialized_hit)
+    return event_factory.new_event('event' => { 'original' => serialized_hit }, 'tags' => ['_elasticsearch_input_failure'])


It is technically redundant, but present for readability.

The explicit return statement makes clear to me that we are intentionally returning that object from the rescue... block, because otherwise when I'm reading code I don't know whether the return value matters to the caller(s).

It's a side-effect of using exceptions for flow-control (which is less than ideal, but it is what we are already starting with).

lib/logstash/inputs/elasticsearch.rb

yaauie · 2024-12-31T00:42:04Z

lib/logstash/inputs/elasticsearch.rb

+    set_docinfo_fields(hit, event) if @docinfo
+
+    event
+  rescue => e


Yes, but this isn't the only exception we're handling here.

We know that EventFactory#new_event (which delegates to Event::new) can give us exceptions too, specifically when the payload uses reserved fields in incorrect ways. We want to catch any situation in which we fail to create an event from the hit.

yaauie · 2024-12-31T00:44:40Z

lib/logstash/inputs/elasticsearch.rb

+
+    event
+  rescue => e
+    serialized_hit = hit.to_json


Ehhh, we deserialized the hit from json, so it should be able to go back?

If we can't, we are at least not creating a new failure scenario (merely failing to handle something that currently already crashes the plugin).

yaauie commented Oct 20, 2022

View reviewed changes

roaksoax added the status:needs-review label Oct 20, 2022

roaksoax assigned yaauie Oct 21, 2022

karenzone self-assigned this Oct 25, 2022

karenzone reviewed Oct 25, 2022

View reviewed changes

karenzone approved these changes Nov 2, 2022

View reviewed changes

roaksoax added status:approved and removed status:needs-review labels Nov 2, 2022

yaauie force-pushed the tolerate-failure branch from da01542 to b42442c Compare November 5, 2024 16:52

yaauie added 4 commits December 26, 2024 18:53

test setup: ensure presence of /etc/protocols

47d44cc

test setup: actually run secure_integration tests

1782d09

When SECURE_INTEGRATION is speicified, the (non-secure) `:integration` specs are excluded, so we cannot have the `:secure_integration` specs wrapped in a context flagged as `:integration`.

test setup: regnerate test certs (and add regen script)

8caeb9f

test setup: give ES the full cert chain

79cfe85

In order for the `ca_trusted_fingerprint` specs to work with the CA's fingerprint, ES needs to be configured to present a cert chain that includes the CA.

yaauie force-pushed the tolerate-failure branch from b42442c to 4a9e689 Compare December 26, 2024 22:49

yaauie changed the base branch from main to 4.x December 26, 2024 22:50

yaauie closed this Dec 26, 2024

yaauie reopened this Dec 26, 2024

yaauie force-pushed the tolerate-failure branch from 4a9e689 to 8912943 Compare December 26, 2024 23:42

add link to changelog

5529740

donoghuc reviewed Dec 30, 2024

View reviewed changes

yaauie commented Dec 31, 2024

View reviewed changes

remove orphan method from refactor

4172d62

yaauie requested a review from donoghuc December 31, 2024 00:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tolerate failure #183

Tolerate failure #183

yaauie commented Oct 20, 2022

yaauie Oct 20, 2022

karenzone Oct 25, 2022

karenzone left a comment

karenzone Oct 25, 2022

karenzone left a comment

donoghuc Dec 30, 2024

donoghuc Dec 30, 2024

yaauie Dec 31, 2024

donoghuc Dec 30, 2024

yaauie Dec 31, 2024

donoghuc Dec 30, 2024

donoghuc Dec 30, 2024

yaauie Dec 31, 2024

donoghuc left a comment

yaauie Dec 31, 2024

yaauie Dec 31, 2024

yaauie Dec 31, 2024


		Common causes are:

		- When the hit result contains top-level fields that are {logstash-ref}/processing.html#reserved-fields[reserved in Logstash] but do not have the expected shape. Use the <<plugins-{type}s-{plugin}-target>> directive to avoid conflicts with the top-level namespace.

	return event_factory.new_event('event' => { 'original' => serialized_hit }, 'tags' => ['_elasticsearch_input_failure'])
	event_factory.new_event('event' => { 'original' => serialized_hit }, 'tags' => ['_elasticsearch_input_failure'])

Tolerate failure #183

Are you sure you want to change the base?

Tolerate failure #183

Conversation

yaauie commented Oct 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karenzone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karenzone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donoghuc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment